-
Notifications
You must be signed in to change notification settings - Fork 35
Test listlabels
#331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test listlabels
#331
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK ebaa961
integration_test/tests/wallet.rs
Outdated
|
||
let json: ListLabels = node.client.list_labels().expect("listlabels"); | ||
|
||
assert!(json.0.contains(&label.to_string())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to ack/merge this but in case it needs a rebase this line is a bit off. The label
var is of type &str
. This line allocates memory, creates an String
, then immediately dereferences it to get an &str
. So we can just pass in label
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha I merged in order of PR number, got all the way to here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
label
var is of type&str
. This line allocates memory, creates anString
, then immediately dereferences it to get an&str
. So we can just pass inlabel
.
So... I had to change it to any()
with a closure. With assert!(json.0.contains(label));
I get the error expected `&String`, found `&str`
which is why I changed it to the above originally. I didn't know that you could compare a &String
to &str
with the ==
in the closure but not with contains()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bizarre, I thought I tested my suggestion yesterday but I guess I did something wrong (probably ran the test17
command from crate root, thereby not actually building the test).
And thanks, TIL about using any
- its literally in the docs of contains
!
ebaa961
to
5d10d17
Compare
`listlabel` was untested and had a model with no bitcoin types. Add a test and remove the model. Update verify, and types.
5d10d17
to
5a756ec
Compare
I'm glad I had to fix this, the Also first rebase with jj, had to do it twice, first time messed up the commands but going back with i.e. I made the change and rebased to fix merge conflicts. |
Mad, you are winning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5a756ec
listlabels
was untested and had a model with no bitcoin types.Add a test and remove the model.
Update verify, and types.